Fix bytewise parameter write acknowledgements - #14662
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes parameter write acknowledgements for bytewise MAVLink parameter types by decoding MAV_PARAM_TYPE_UINT8/UINT16 (and similarly INT8/INT16) into QGC’s canonical 32-bit QVariant integer representations, preventing valid PARAM_VALUE echoes from being rejected due to mismatched QVariant types.
Changes:
- Fix
_mavlinkParamUnionToVariantto cast narrow integer types toqint32/quint32before wrapping inQVariant. - Add unit tests covering successful writes of
UINT8andUINT16parameters. - Extend
MockLinkto load a minimal generic parameter set containingUINT8/UINT16test parameters.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| test/FactSystem/ParameterManagerTest.h | Adds new unit test slots and extends the helper signature to parameterize param name and autopilot type. |
| test/FactSystem/ParameterManagerTest.cc | Implements UINT8/UINT16 write tests and updates the helper to support generic autopilot + integer step sizing. |
| src/FactSystem/ParameterManager.cc | Fixes UINT8/UINT16 (and INT8/INT16) decoding to consistent 32-bit QVariant integer types. |
| src/Comms/MockLink/MockLink.cc | Loads a new generic parameter file when MAV_AUTOPILOT_GENERIC is used. |
| src/Comms/MockLink/GenericMockLink.params | Adds test parameters TEST_UINT8 and TEST_UINT16 with MAVLink types for coverage. |
| src/Comms/MockLink/CMakeLists.txt | Registers GenericMockLink.params as a Qt resource for the mock link. |
| return true; | ||
| case MAV_PARAM_TYPE_UINT8: | ||
| outValue = QVariant(paramUnion.param_uint8); | ||
| outValue = QVariant(static_cast<quint32>(paramUnion.param_uint8)); |
There was a problem hiding this comment.
n.b. Without this cast, QVariant ends up storing an int, not a uint, while the fact system we compare this value to stores a uint. Due to the mismatch, QGC rejects write acknowledgement, retries and eventually shows the write failure.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #14662 +/- ##
==========================================
+ Coverage 25.47% 32.55% +7.08%
==========================================
Files 769 782 +13
Lines 65912 67259 +1347
Branches 30495 31164 +669
==========================================
+ Hits 16788 21897 +5109
+ Misses 37285 30608 -6677
- Partials 11839 14754 +2915
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 472 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
Build ResultsPlatform Status
All builds passed. Pre-commit
Pre-commit hooks: 0 passed, 0 failed, 0 skipped. Test Resultslinux-coverage-integration: 36 passed, 0 skipped Code CoverageCoverage: 69.2% No baseline available for comparison Artifact Sizes
Updated: 2026-07-25 13:49:04 UTC • Commit: 93a7608 • Triggered by: Android |
941ac1c to
d03b7da
Compare
d03b7da to
23252a5
Compare
23252a5 to
bea8d30
Compare
|
Fantastic stuff. Thanks for the fix. Can you rebase to latest upstream master? This should fix the CI failure. Just want to make sure. |
When QGC writes a parameter, it waits for the vehicle to echo the new value in PARAM_VALUE. The acknowledgement is accepted only when both the value and the QVariant type match QGC's stored Fact value. For 8- & 16-bit parameters, FactMetaData stores the value in a QVariant: Qt Int (32-bit) for signed values and 32-bit Qt UInt (32-bit) for unsigned values. This preserves the parameter's signedness in QGC's internal representation. The `_mavlinkParamUnionToVariant` method passed `uint8_t` and `uint16_t` directly to QVariant. C++ selected QVariant's `int` constructor, so an unsigned PARAM_VALUE was decoded as signed Qt Int. The type check then rejected the otherwise valid acknowledgement and the write timed out. Decode narrow values into QGC's canonical Int or UInt representations. The bug affects only UINT8 and UINT16 parameters; the other supported types coincidentally work. It flew below the radar because ArduPilot uses float-based parameter encoding, while PX4 does not expose UINT8 or UINT16 parameters for which the problem exhibits itself.
bea8d30 to
93a7608
Compare
|
@DonLakeFlyer Just rebased and CI is green now 👍 |
Description
Note
This fixes a regression introduced between Stable_V5.0 and Daily, where Daily unintentionally dropped support for MAVLink parameters of type
MAV_PARAM_TYPE_UINT8andMAV_PARAM_TYPE_UINT16.When QGC writes a parameter, it waits for the vehicle to echo the new value in PARAM_VALUE. The acknowledgement is accepted only when both the value and the QVariant type match QGC's stored Fact value.
For 8- & 16-bit parameters, FactMetaData stores the value in a QVariant: Qt Int (32-bit) for signed values and 32-bit Qt UInt (32-bit) for unsigned values. This preserves the parameter's signedness in QGC's internal representation.
The
_mavlinkParamUnionToVariantmethod passeduint8_tanduint16_tdirectly to QVariant. C++ selected QVariant'sintconstructor, so an unsigned PARAM_VALUE was decoded as signed Qt Int. The type check then rejected the otherwise valid acknowledgement and the write timed out. Decode narrow values into QGC's canonical Int or UInt representations.The bug affects only UINT8 and UINT16 parameters; the other supported types coincidentally work. It flew below the radar because ArduPilot uses float-based parameter encoding, while PX4 does not expose UINT8 or UINT16 parameters for which the problem exhibits itself.
Type of Change
Testing
Platforms Tested
Flight Stacks Tested
Screenshots
Checklist
Related Issues
By submitting this pull request, I confirm that my contribution is made under the terms of the project's dual license (Apache 2.0 and GPL v3).